Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add write_dir argument to csv_to_wfdb. Fixes #67. #492

Merged
merged 4 commits into from
Jul 11, 2024

Conversation

tompollard
Copy link
Member

@tompollard tompollard commented Jul 1, 2024

As discussed in #490,

def csv_to_wfdb(
currently "strips the path from the input .csv, then writes the output to .dat and .hea".

It's inconvenient not to be able to specify the output directory. This pull request adds a new output_dir argument to the csv_to_wfdb function. By default output_dir is set to None, which will maintain backwards compatibility. Setting output_dir to a directory will mean that output files are saved to this directory.

I have set this to a WIP, because I haven't tested the new behaviour (other than running pytest). @jshaffer94247, if you have an opportunity to test the fix, I'd appreciate your feedback.

@tompollard tompollard force-pushed the tp/csv_to_wfdb_path branch 2 times, most recently from d6d5aa7 to caff031 Compare July 1, 2024 19:36
@tompollard
Copy link
Member Author

tompollard commented Jul 2, 2024

Tests are failing on an unrelated issue, presumably related to an update to Numpy (e.g. this? https://numpy.org/devdocs/release/1.24.0-notes.html#conversion-of-out-of-bound-python-integers)

@bemoody
Copy link
Collaborator

bemoody commented Jul 2, 2024

It should be write_dir to be consistent with wrsamp, wrann, etc.

The existing logic is pretty broken, though! split(os.sep) is decidedly wrong for Windows, but also replace(".csv", "") is screwy. You want to take the basename and then remove anything after a dot.

@tompollard tompollard changed the title [WIP] Add output_dir argument to csv_to_wfdb. Fixes #67. [WIP] Add write_dir argument to csv_to_wfdb. Fixes #67. Jul 3, 2024
@tompollard tompollard force-pushed the tp/csv_to_wfdb_path branch from 8463fe6 to d9f4e3c Compare July 3, 2024 18:30
@bemoody
Copy link
Collaborator

bemoody commented Jul 3, 2024

No test cases - please add a test case so we can see that this function works.

@tompollard tompollard force-pushed the tp/csv_to_wfdb_path branch from d9f4e3c to 626307a Compare July 9, 2024 19:59
@tompollard tompollard force-pushed the tp/csv_to_wfdb_path branch from 626307a to e6b3b69 Compare July 9, 2024 20:45
…r earlier versions.

Tests are failing on the test-deb10-i386 build because it is running an old version of Pandas.
@tompollard tompollard changed the title [WIP] Add write_dir argument to csv_to_wfdb. Fixes #67. Add write_dir argument to csv_to_wfdb. Fixes #67. Jul 9, 2024
@tompollard
Copy link
Member Author

@bemoody this is now ready to review.

@bemoody
Copy link
Collaborator

bemoody commented Jul 11, 2024

pandas/core/base.py says "tolist is not deprecated" (to_list is an alias for tolist), so can we simply use tolist?

@tompollard
Copy link
Member Author

pandas/core/base.py says "tolist is not deprecated" (to_list is an alias for tolist), so can we simply use tolist?

I considered this but decided to use to_list() because it sounds like it is what the developers would like us to use (I think for consistency with other pandas methods). I think dropping the try/except is fine though...now done.

Copy link
Collaborator

@bemoody bemoody left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks - looks good!

@tompollard
Copy link
Member Author

Thanks Benjamin

@tompollard tompollard merged commit 6a0de80 into main Jul 11, 2024
14 checks passed
@tompollard tompollard deleted the tp/csv_to_wfdb_path branch July 11, 2024 18:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants